API: Add compatibility checks for Schemas with default values #11434
Conversation
|
|
||
| @ParameterizedTest | ||
| @ValueSource(ints = {1, 2, 3}) | ||
| public void testSupportedInitialDefault(int formatVersion) { |
There was a problem hiding this comment.
testSupportedWriterDefault?
There was a problem hiding this comment.
| public void testSupportedInitialDefault(int formatVersion) { | |
| public void testSupportedWriterDefault(int formatVersion) { |
| @ParameterizedTest | ||
| @ValueSource(ints = {1, 2, 3}) | ||
| public void testSupportedInitialDefault(int formatVersion) { | ||
| // only the initial default is a forward-incompatible change |
There was a problem hiding this comment.
nit: We could make this self contained to the test by saying
Writer defaults are universally compatible
Or something?
|
Tests failing are because Metadata.json also has validations for this and this check is failing first |
| problems.put( | ||
| field.fieldId(), | ||
| String.format( | ||
| "Invalid type for %s: %s is not supported until v%s", |
There was a problem hiding this comment.
Existing checks in TableMetadata look like
"Invalid type in v%s schema: struct.ts_nanos timestamptz_ns is not supported until v3"
|
Updated. Thanks, @RussellSpitzer! |
Co-authored-by: Fokko Driesprong <[email protected]>
| @@ -586,16 +587,37 @@ private List<NestedField> reassignIds(List<NestedField> columns, TypeUtil.GetID | |||
| * @param formatVersion table format version | |||
| */ | |||
| public static void checkCompatibility(Schema schema, int formatVersion) { | |||
| // check the type in each field | |||
| // accumulate errors as a treemap to keep them in a reasonable order | |||
| Map<Integer, String> problems = Maps.newTreeMap(); | |||
There was a problem hiding this comment.
[optional] if we are going for another revision as fieldId() is int :
| Map<Integer, String> problems = Maps.newTreeMap(); | |
| Map<int, String> problems = Maps.newTreeMap(); |
There was a problem hiding this comment.
Primitives are not allowed as Map Keys :(
There was a problem hiding this comment.
Or any type parameter I think ... I feel like this is a 1337code question
|
Thanks everyone - @rdblue (for original fix), @Fokko , @anuragmantri , @kevinjqliu , @singhpk234 , @amogh-jahagirdar For reviews! |
…#11434) Co-authored-by: Russell Spitzer <[email protected]> Co-authored-by: Fokko Driesprong <[email protected]>
This updates
Schema.checkCompatibility:initial-defaultis not set for v1 and v2 tables, which would break forward compatibilitycheckCompatibilityinTestSchemaI think that this should get into 1.7.0 to avoid accidentally leaking initial defaults in v1 and v2 tables.